-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-13746][Tests]stop using deprecated SynchronizedSet #11580
Conversation
toBeCleanedShuffleIds.isEmpty && | ||
toBeCleanedBroadcstIds.isEmpty && | ||
toBeCheckpointIds.isEmpty | ||
synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't synchronizing access to these sets on the same lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen
Fixed. Thanks!
val toBeCleanedShuffleIds = new HashSet[Int] with SynchronizedSet[Int] ++= shuffleIds | ||
val toBeCleanedBroadcstIds = new HashSet[Long] with SynchronizedSet[Long] ++= broadcastIds | ||
val toBeCheckpointIds = new HashSet[Long] with SynchronizedSet[Long] ++= checkpointIds | ||
val toBeCleanedRDDIds = new HashSet[Int] ++= rddIds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just be = HashSet(rddIds)
and similarly for the next 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen
I tried to change to val toBeCleanedRDDIds = HashSet(rddIds), but I got error at
toBeCleanedRDDIds -= rddId
so I will keep val toBeCleanedRDDIds = new HashSet[Int] ++= rddIds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right Scala doesn't have a constructor that builds a set from a list/seq, so this makes a set of a Seq[Int]
. Yes this is fine.
This needs a rebase anyway so I left a few more comments. Otherwise LGTM pending tests |
ok to test |
Test build #52968 has finished for PR 11580 at commit
|
Test build #53024 has finished for PR 11580 at commit
|
@@ -578,18 +578,27 @@ class CleanerTester( | |||
} | |||
|
|||
private def uncleanedResourcesToString = { | |||
val s1 = {toBeCleanedRDDIds.synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two statements have extra braces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srowen
Sorry, I overlooked this. Will change now. Thanks!
Test build #53035 has finished for PR 11580 at commit
|
Merged to master |
trait SynchronizedSet in package mutable is deprecated Author: Wilson Wu <wilson888888888@gmail.com> Closes apache#11580 from wilson888888888/spark-synchronizedset.
trait SynchronizedSet in package mutable is deprecated Author: Wilson Wu <wilson888888888@gmail.com> Closes apache#11580 from wilson888888888/spark-synchronizedset.
trait SynchronizedSet in package mutable is deprecated